Fix billboard on terrain and Globe.getHeight #4622

duvifn · 2016-11-07T20:29:45Z

better handling of under ellipsoid surface terrain

duvifn · 2016-11-07T20:47:52Z

I had a typo in the commit message (Globe.pick instead of Globe.getHeight). So I made commit --amned and now it's ready

mramato · 2016-11-07T20:51:36Z

Thanks @duvifn! This bug has been on my wish list for quite some time. @bagnell can you please review and merge, we already have a CLA.

duvifn · 2016-11-08T10:32:16Z

Source/Scene/Globe.js

+        var surfaceNormal = ellipsoid.geodeticSurfaceNormal(cartesian, ray.direction);
+
+        // compute origin point, to account for a case where the terrain is under ellipsoid surface
+        var minimumHeight = Math.min(defaultValue(tile.data.minimumHeight, 0.0), 0.0);


@bagnell , do you think it would be safer to use lower value instead of 0.0 in defaultValue(tile.data.minimumHeight, 0.0)?
for example -11500.0 (taken from here, wikipedia)

I would use -11500.0. For the lowest resolution terrain tiles, triangle vertices may be near the ellipsoid surface, but the interiors cutting through the ellipsoid will drop to around this height. Is there a reason you didn't use Cartesian3.ZERO? Did you run into precision issues in the triangle intersection test?

bagnell · 2016-11-21T19:55:00Z

This looks good to me. Just that one comment.

pjcozzi · 2016-11-21T20:33:52Z

Also, please update CHANGES.md.

…he z-axis

duvifn · 2016-11-27T16:00:11Z

Thanks @bagnell!

Is there a reason you didn't use Cartesian3.ZERO

A ray from Cartesian3.ZERO with Surface Normal as a direction doesn't pass through the given cartographic:

The normal at a point on the surface of an ellipsoid does not pass through the centre, except for points on the equator or at the poles

Anyway, I discussed this problem with Omer Bar from Survey Of Israel/Geodesy department and he suggested to use the intersection point of the Surface Normal with z axis (x = 0 , y = 0, z = <varying value>).
Our Ellipsoid is a surface of revolution, so surface normal intersects the axis of rotation.

Theoretically, the intersection point can be outside the ellipsoid, though I really tried but didn't get such result.
For any of such edge cases, we can check if abs(z) is less than ellipsoid.minimumRadius with some buffer. If it's outside the ellipsoid we can use the min of tile.minimumHeight / -11500 .

If you want to see how this intersection point behaves, see this sandcastle example that shows how this point is changed relative to the normal.
The label is the intersection point's values, the arrow is the surface normal.
Click Toggle animation to start.

Do you see any problem with this approach?
If no,
I changed the code according to this suggestion. Can you review it?

BTW,
I suspect (not certain, though) that some strange behavior of the camera under terrain (#4680, #4660) should be improved with this PR since Camera.prototype._adjustHeightForTerrain uses Globe.getHeight (that currently returns a wrong result or undefined near the surface).

Conflicts: CHANGES.md

duvifn · 2016-12-05T11:29:02Z

I changed the code a bit according to @pjcozzi 's suggestion in #4682.
Now cartesian is computed on the ellipsoid surface.

@bagnell, I didn't find any other usage of data.position (changed to data.positionOnEllipsoidSurface) in the code that could be impacted by this change, but it's possible I missed something.

This is ready for review.

pjcozzi · 2016-12-05T22:59:18Z

Source/Scene/Globe.js

+        Cartesian3.subtract(cartesian, vectorToMinimumPoint, ray.origin);
+
+        // Theoretically, the intersection point can be outside the ellipsoid, so we have to check if the result's 'z' is inside the ellipsoid (with some buffer)
+        if (Math.abs(ray.origin.z) >= ellipsoid.radii.z -11500.0){


Instead of hard coding -11500.0 here and below, can this use some percentage of ellipsoid.radii.maximumComponent? Or does the buffer need to be this big? Instead, can it be a much smaller epsilon?

pjcozzi · 2016-12-05T23:00:29Z

Source/Scene/QuadtreePrimitive.js

                    }

                    if (mode === SceneMode.SCENE3D) {
-                        Cartesian3.clone(Cartesian3.ZERO, scratchRay.origin);
-                        Cartesian3.normalize(data.position, scratchRay.direction);
+                        var surfaceNormal = ellipsoid.geodeticSurfaceNormal(data.positionOnEllipsoidSurface, scratchRay.direction);


Does this duplicate code belong in a utility function for Ray or Ellipsoid? That would also be more testable so we could add unit tests.

bagnell · 2016-12-07T18:34:59Z

A ray from Cartesian3.ZERO with Surface Normal as a direction doesn't pass through the given cartographic

Ah, of course. I can't see anything wrong with your new approach. This looks good to me. @pjcozzi can merge when this is ready.

change Globe.js and QuadtreePrimitive.js to work with Ellipsoid.getSurfaceNormalIntersectionWithZAxis add tests

duvifn · 2016-12-12T16:20:16Z

Hi,
Thanks for reviewing this.

Instead of hard coding -11500.0 here and below, can this use some percentage of ellipsoid.radii.maximumComponent? Or does the buffer need to be this big? Instead, can it be a much smaller epsilon?

This value was taken from cesium code, here.

@bagnell wrote about this value:

I would use -11500.0. For the lowest resolution terrain tiles, triangle vertices may be near the ellipsoid surface, but the interiors cutting through the ellipsoid will drop to around this height.

For this value to be be calculated accurately we need:
1) width/height of tiles at the lowest level.
2) ellipsoid dimensions
3) terrain provider minimum possible value
It is also important to compute this value only once in the terrain provider initialization so we wouldn't have to calculate it every time we do some operations.

To make it possible we have to do some code refactoring, and I think that this is out of the scope of this PR.
I think @bagnell wrote the following comment in the code because of the above reasons:

// minimum height for the terrain set, need to get this information from the terrain provider

Anyway, for Earth datums all of this is not needed since the intersection point with the z-axis would always be very close to Cartesian3.ZERO.

After I tested it practically, I asked a mathematician colleague to prove that formally and I put here the proof for that (I had to upload it as a pictures because MD doesn't support latex).

For Earth (WGS84 ellipsoid), the absolute value of this constant is 0.00673 (i.e. point is always very close to Cartesian3.ZERO);

For Moon, according to wikipedia values, the constant is 0.0024.

I don't really know other shapes of planetary bodies that cesium might represent in the future, hence there is the fallback we discussed above.

Does this duplicate code belong in a utility function for Ray or Ellipsoid? That would also be more testable so we could add unit tests.

I added Ellipsoid.prototype.getSurfaceNormalIntersectionWithZAxis function with a bit optimized code (multiply only the z by a constant instead of some vector operations), and wrote specs for it.

This is ready.

…SquaredZ

duvifn · 2016-12-13T20:45:28Z

Minor changes.
This is ready.

CC: @pjcozzi @bagnell

pjcozzi · 2016-12-24T21:20:06Z

Source/Core/Ellipsoid.js

+    Ellipsoid.prototype.getSurfaceNormalIntersectionWithZAxis = function(position, buffer, result) {
+
+        //>>includeStart('debug', pragmas.debug);
+        if (!defined(position)) {


Really minor, but could you please add unit tests for these three DeveloperError cases?

pjcozzi · 2016-12-24T21:25:24Z

Thanks for the updates, @duvifn!

I made some trivial style tweaks for consistency with the rest of the Cesium codebase. Let me know when you add those other unit tests (it will be easy) and we'll merge this. If we merge it early next week, it will ship in Cesium 1.29 on January 2.

duvifn · 2016-12-25T12:11:55Z

Hi @pjcozzi !

Did you mean these specs? (already there),
Or I didn't correctly understand you?

pjcozzi · 2016-12-26T21:35:14Z

Ah yes, great!

Thanks again for this contribution!

duvifn added 2 commits November 2, 2016 21:46

fix_billboardOnTerrain

ead79f4

fix Globe.getHeight function

9f7d911

better handling of under ellipsoid surface terrain

duvifn force-pushed the fix_billboardOnTerrain branch from b2dc7c6 to 9f7d911 Compare November 7, 2016 20:43

duvifn commented Nov 8, 2016

View reviewed changes

duvifn mentioned this pull request Nov 24, 2016

Ellipsoid.geodeticSurfaceNormal returns wrong result if the cartesian is not on the ellpsoid surface #4682

Open

duvifn added 3 commits November 27, 2016 14:50

compute origin point at the intersection of the surface normal with t…

156d5bb

…he z-axis

add to CHANGES.md

4d12c0e

typos

9f551ab

duvifn mentioned this pull request Nov 27, 2016

Billboards on terrain - some problems #4686

Open

duvifn added 2 commits December 5, 2016 10:32

Merge branch 'master' into fix_billboardOnTerrain

306c276

Conflicts: CHANGES.md

cartesian is computed on the ellipsoid surface

74a7de5

duvifn force-pushed the fix_billboardOnTerrain branch from a256f7f to 74a7de5 Compare December 5, 2016 11:01

fix typo

30e73ff

pjcozzi reviewed Dec 5, 2016

View reviewed changes

added Ellipsoid.getSurfaceNormalRayFromZAxis. no tested

7894d20

duvifn added 2 commits December 12, 2016 17:23

fix Ellipsoid.getSurfaceNormalIntersectionWithZAxis

edfd685

change Globe.js and QuadtreePrimitive.js to work with Ellipsoid.getSurfaceNormalIntersectionWithZAxis add tests

Merge branch 'master' into fix_billboardOnTerrain

1b4f97d

duvifn added 4 commits December 13, 2016 11:05

factor out sqauredAOverSquaredB

23d2fea

add _sqauredAOverSquaredB to the Ellipsoid constructor

dbd917d

add check if radii.z===0

0947e12

remove unused dependency, change sqauredAOverSquaredB to sqauredXOver…

244cc52

…SquaredZ

hpinkos mentioned this pull request Dec 21, 2016

CLAMP_TO_GROUND for Billboards incorrect positions #4776

Open

pjcozzi reviewed Dec 24, 2016

View reviewed changes

pjcozzi added 5 commits December 24, 2016 16:21

Whitespace

9ae322c

Whitespace

056fc20

Style tweak

b394335

Style tweak

1f325e5

Style tweak

761eda4

fix typo in spec title

d01196b

pjcozzi merged commit 38ed8b2 into CesiumGS:master Dec 26, 2016

pjcozzi mentioned this pull request Dec 26, 2016

globe.getHeight randomly returns 'undefined' #3411

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix billboard on terrain and Globe.getHeight #4622

Fix billboard on terrain and Globe.getHeight #4622

duvifn commented Nov 7, 2016

duvifn commented Nov 7, 2016

mramato commented Nov 7, 2016

duvifn Nov 8, 2016 •

edited

Loading

bagnell Nov 21, 2016

bagnell commented Nov 21, 2016

pjcozzi commented Nov 21, 2016

duvifn commented Nov 27, 2016 •

edited

Loading

duvifn commented Dec 5, 2016

pjcozzi Dec 5, 2016

pjcozzi Dec 5, 2016

bagnell commented Dec 7, 2016

duvifn commented Dec 12, 2016 •

edited

Loading

duvifn commented Dec 13, 2016

pjcozzi Dec 24, 2016

pjcozzi commented Dec 24, 2016

duvifn commented Dec 25, 2016

pjcozzi commented Dec 26, 2016

Fix billboard on terrain and Globe.getHeight #4622

Fix billboard on terrain and Globe.getHeight #4622

Conversation

duvifn commented Nov 7, 2016

duvifn commented Nov 7, 2016

mramato commented Nov 7, 2016

duvifn Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

bagnell Nov 21, 2016

Choose a reason for hiding this comment

bagnell commented Nov 21, 2016

pjcozzi commented Nov 21, 2016

duvifn commented Nov 27, 2016 • edited Loading

duvifn commented Dec 5, 2016

pjcozzi Dec 5, 2016

Choose a reason for hiding this comment

pjcozzi Dec 5, 2016

Choose a reason for hiding this comment

bagnell commented Dec 7, 2016

duvifn commented Dec 12, 2016 • edited Loading

duvifn commented Dec 13, 2016

pjcozzi Dec 24, 2016

Choose a reason for hiding this comment

pjcozzi commented Dec 24, 2016

duvifn commented Dec 25, 2016

pjcozzi commented Dec 26, 2016

duvifn Nov 8, 2016 •

edited

Loading

duvifn commented Nov 27, 2016 •

edited

Loading

duvifn commented Dec 12, 2016 •

edited

Loading